BUG: reject live RNG objects as Sensor seeds - #1174
Conversation
e8c43e8 to
6ac4ad9
Compare
thc1006
left a comment
thc1006
left a comment
There was a problem hiding this comment.
Requesting changes for two correctness gaps:
RandomStateis accepted bydefault_rngon NumPy >= 2.2, so the original “constructs successfully, fails later at JSON serialization” bug is still reachable on a supported RocketPy dependency version.- Accepted mutable seed descriptors are stored by reference. A later mutation can make
to_dict()serialize a seed that no longer describes the stream used to initialize the sensor. This affects list/ndarray seeds andSeedSequenceinstances backed by mutable entropy.
I would also align the annotation and error text with the actual array-like integer contract (np.integer and ndarray are accepted today), and make the round-trip tests compare the generated noise stream rather than only the stored seed/state.
The constructor-level rejection is the right direction; these changes would make the boundary complete rather than covering only two currently known live RNG classes.
| # entropy and spawn key; a live generator has no such description. | ||
| # Without this check the sensor builds fine and only fails at | ||
| # json.dumps(), far from the call that caused it. | ||
| if isinstance(seed, (np.random.Generator, np.random.BitGenerator)): |
There was a problem hiding this comment.
np.random.default_rng also accepts np.random.RandomState starting with NumPy 2.2. RocketPy supports numpy>=1.23 with no upper bound, so this still leaves the same late-failure path on current NumPy: the sensor constructs, the RandomState remains live state, and RocketPyEncoder cannot encode it.
Could we reject RandomState here as well and add it to the parametrized rejection test? More generally, validating against the stable seed-descriptor contract (SeedSequence entropy inputs) would be less brittle than blacklisting whichever live RNG types default_rng happens to accept today.
|
Thanks — the
So the check now accepts descriptors instead of naming live types: Worth noting for anyone tightening this later: Annotation. Widened to what the check actually takes, via a Round-trip tests. Agreed, comparing the stored seed proves too little. Added a test that draws from the restored sensor and compares the sequence against a fresh sensor built from the same seed, parametrized over int, Mutable seed descriptors. Confirmed, and it is worse than only the seed field: seed = [1, 2, 3]
sensor = Accelerometer(sampling_rate=10, seed=seed)
seed[0] = 999
sensor.to_dict()["seed"] # [999, 2, 3]
I've left it out of this PR deliberately. Local run: |
|
No workflow has run on this PR at all — not on the review commits, not on the first one. The three check suites are still sitting at |
`Sensor.__init__` passes the seed straight to `numpy.random.default_rng`, which also accepts `Generator` and `BitGenerator` objects. The sensor then constructs successfully and stores the object on `self._seed`, where `to_dict()` emits it verbatim, so the failure only surfaces later at `json.dumps()`, far from the call that caused it. RocketPy-Team#1124 closed the `SeedSequence` case in RocketPy-Team#1087 by teaching `RocketPyEncoder` to write one out. That works because a `SeedSequence` is defined by its entropy and spawn key, so it still describes the stream after a round trip. A `Generator` has no such description: its state advances on every draw, so whatever `to_dict()` wrote would depend on when it ran, and restoring it would not reproduce the stream the sensor actually used. Reject those two in the constructor instead, so the failure stays at the call that caused it. Ints, numpy ints, `SeedSequence` and `None` are untouched, as are the sequences of ints `default_rng` accepts and the encoder already serializes, so no seed that works today is rejected. Annotate `seed` on every constructor that takes one, with the type the issue itself names, so the contract is stated where the argument is declared rather than only in the docstring.
default_rng also accepts RandomState from NumPy 2.2 on, and RocketPy pins no upper bound on numpy, so the previous isinstance list let it through to the same late TypeError at json.dumps() that RocketPy-Team#1087 reported. Check the stable half of the contract instead of enumerating the live types: accept ints, array_like of ints and SeedSequence, and refuse the rest. A seed kind numpy starts accepting later is now refused at construction rather than reaching serialization. Widen the annotation to the array_like integer contract the check actually takes. It goes through a SeedLike union so the seven signatures stay inside the line limit while help() and inspect.signature() still expand the members.
The existing round-trip tests assert on the stored seed value, which would still pass for a seed that survives JSON without naming the stream the original sensor used. Draw from the restored sensor instead and compare it against a fresh one built from the same seed, across the four descriptor kinds the constructor accepts.
5c079d2 to
715bfa1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1174 +/- ##
===========================================
+ Coverage 91.13% 91.14% +0.01%
===========================================
Files 131 131
Lines 17560 17576 +16
===========================================
+ Hits 16003 16020 +17
+ Misses 1557 1556 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The seed check accepted a nested sequence of ints, on the stated grounds that "numpy accepts as entropy just the same". It does not: SeedSequence raises TypeError for a nested sequence and ValueError for an array of two or more dimensions. Older NumPy let the nested form through, which is why this passed under Python 3.10 and failed under 3.14, where a newer NumPy is resolved -- the acceptance was never portable, and RocketPy sets no upper bound on the dependency. So _is_int_array_like now takes a flat sequence only, and an ndarray only at ndim <= 1. Both refusals name the seed, where the NumPy messages they replace name neither it nor the argument that carried it. The nested case moves out of test_int_array_like_seeds_are_accepted and into a rejection test alongside a 2-D array, which the check would have admitted for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Summary
Sensor.__init__hands the seed tonumpy.random.default_rng, which also acceptsGeneratorandBitGeneratorobjects. The sensor constructs,to_dict()emits the object verbatim, and the failure surfaces later atjson.dumps():This is the case #1124 leaves behind. #1124 closed the
SeedSequencehalf of #1087 by teachingRocketPyEncoderto write one out, which works because aSeedSequenceis defined by its entropy and spawn key and still describes the stream after a round trip.A
Generatorhas no such description. Its state advances on every draw, so whatto_dict()wrote would depend on when it ran, and a restored copy would not reproduce the stream the sensor used. There is nothing to serialize it to, so it is rejected at the constructor instead, where the caller can still see which argument was wrong.What changes
GeneratorandBitGeneratorseeds raiseTypeErrorat construction.seedis annotated asint | Sequence[int] | np.random.SeedSequence | Noneon every constructor that takes one — the type this issue names in its own wording — so the contract is stated where the argument is declared and not only in the docstring. The docstrings now match.SeedSequence,None, and the sequences of intsdefault_rngaccepts all behave exactly as they do on develop today.Test plan
pytest tests/unit/sensors/ -q→ 63 passed, including BUG: serialize numpy SeedSequence for sensor seeds (#1087) #1124'stest_seedsequence_sensor_seed_is_json_serializablepytest tests/unit -q→ no new failures (the 4test_sensitivity.pyerrors are a missingstatsmodelsin my environment and reproduce on a cleandevelop)Generator/BitGeneratorrejected ·SeedSequenceand a sequence of ints explicitly still accepted ·None/0/int/np.int64/2**128-1still accepted and round trip